Skip to content

Harden fail-closed handling, fix mutex leaks, and add mutation-verified tests#84

Open
JeremiahM37 wants to merge 4 commits into
wolfSSL:masterfrom
JeremiahM37:fenrir-fail-closed-hardening
Open

Harden fail-closed handling, fix mutex leaks, and add mutation-verified tests#84
JeremiahM37 wants to merge 4 commits into
wolfSSL:masterfrom
JeremiahM37:fenrir-fail-closed-hardening

Conversation

@JeremiahM37
Copy link
Copy Markdown
Contributor

@JeremiahM37 JeremiahM37 commented May 11, 2026 •
edited
Loading

Fixes F-2060, F-2061, F-2438, F-2445, F-2446, F-2456, F-2458, F-2459, F-2460, F-2461, F-2464, F-2465, F-2837, F-2838, F-3866, F-3867, F-3869, F-3873

  • Unlock mutex on early-return failure paths in wolfsentry_event_update_config and wolfsentry_event_set_aux_event
  • Bound JSON prefix-bits to the address family max to prevent OOB read in wolfsentry_route_init
  • Reject PENALTYBOXED flag in route_flags_to_add_on_insert and route_flags_to_clear_on_insert, and run flag sensicality check on both
  • Return ERR_ABRT instead of ERR_OK when the lwIP filter callbacks see a NULL wolfsentry context
  • Treat dispatch errors as REJECT in the lwIP ethernet/IPv4/IPv6/TCP/ICMP4 filters
  • Treat dispatch errors as REJECT in the lwIP UDP filter
  • Treat dispatch errors as REJECT in the lwIP ICMPv6 filter
  • Treat dispatch errors as deny in wolfip_dispatch_event
  • Deny unrecognized protocols and event reasons in the wolfIP filter shims
  • Return ERR_ABRT for "can't happen" filter event reasons in the lwIP shims
  • Extend the user-value mutability test to assert that overwrite_p=1 is rejected on a read-only KV
  • New tests
    • derogatory_threshold_for_penaltybox == 0 disables auto-penaltybox
    • penaltybox_duration == 0 means permanent penaltybox
    • penaltybox-timeout boundary semantics (uses a mock clock)
    • DONT_COUNT_CURRENT_CONNECTIONS flag bypasses the connection-count cap
    • INHIBIT_ACTIONS flag suppresses action dispatch
    • action_res_filter_bits_unset route-exclusion logic
    • stale-purge boundary semantics (uses a mock clock)

@JeremiahM37 JeremiahM37 self-assigned this May 11, 2026
@JeremiahM37 JeremiahM37 force-pushed the fenrir-fail-closed-hardening branch from de383b6 to 451b106 Compare May 12, 2026 16:55
@JeremiahM37 JeremiahM37 assigned douzzer and unassigned JeremiahM37 and douzzer May 12, 2026
douzzer
douzzer requested changes May 15, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@douzzer douzzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a couple false positives in the mix here -- the mitigations for them need to be reverted:

F-2836 "route_flags_to_add_on_insert accepts GREENLISTED flag, enabling auto-accept of all inserted routes"

F-2840 "route_flags_to_clear_on_insert accepts PENALTYBOXED flag without validation, bypassing penalty boxing for inserted routes"

I've closed the Fenrir issues as WontFix.

@douzzer
Copy link
Copy Markdown
Collaborator

douzzer commented May 15, 2026

Also it would be good to have dispatch errors in the glue layer invoke the default policy, though that would be tricky. E.g. clearly there's no way to discover the default policy if the wolfsentry context is null -- ABRT seems reasonable there. But when there's a context to check, we should use its default.

@JeremiahM37 JeremiahM37 force-pushed the fenrir-fail-closed-hardening branch from 451b106 to be38b34 Compare May 17, 2026 00:17
@JeremiahM37
Copy link
Copy Markdown
Contributor Author

JeremiahM37 commented May 17, 2026

Reverted the changes for F-2836 and F-2840. Glue dispatch failures now consult the context's configured default policy via wolfsentry_route_default_policy_get (under a shared lock), rejecting only if that policy rejects. If the policy can't be determined _ NULL context, lock failure, or getter failure _ it fails closed (ABRT / -WOLFIP_EACCES), matching the null-context case you flagged; the pre-existing null-context guards still ABRT before dispatch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@douzzer douzzer douzzer requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@douzzer douzzer

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@JeremiahM37 @douzzer